build: Reuse //base:build_date_internal for //copied_base/base:build_date#10021
build: Reuse //base:build_date_internal for //copied_base/base:build_date#10021hferreiro wants to merge 1 commit intoyoutube:mainfrom
Conversation
…date https://crrev.com/c/6551784 and https://crrev.com/c/6546841 modified //base:build_date to simply copy the generated timestamp header file from the default toolchain and avoid having to redo the work in every toolchain. The changes also introduced an assertion to ensure that was the case. In AOSP builds, that assertion fails because it uses copied_base with the old code. This PR fixes this by reusing the same mechanism that is already in place at //base:build_date_internal. Bug: 495203133
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies copied_base/base/BUILD.gn to replace the build_date action with a copy target that reuses a header generated by //base:build_date_internal. It also adds the generated header to the base component's sources. A review comment suggests using get_label_info to dynamically resolve the source path instead of using a hardcoded string, which improves maintainability and adheres to GN best practices.
| ] | ||
| copy("build_date") { | ||
| # Reuse the file generated by //base:build_date_internal. | ||
| sources = [ "$root_build_dir/gen/base/generated_build_date_internal.h" ] |
There was a problem hiding this comment.
Using a hardcoded path for the source file is brittle as it assumes a specific directory structure within the build output. It is more robust to use get_label_info to dynamically determine the output directory of the dependency, especially when referencing a target in a different toolchain. This aligns with Chromium GN best practices for maintainability.
| sources = [ "$root_build_dir/gen/base/generated_build_date_internal.h" ] | |
| sources = [ get_label_info("//base:build_date_internal($default_toolchain)", "target_gen_dir") + "/generated_build_date_internal.h" ] |
References
- Adhere to established upstream Chromium style guides for GN files, which prefer using get_label_info for cross-target path references to avoid brittle hardcoded paths. (link)
There was a problem hiding this comment.
This uses the same sources that //base:build_date uses.
1 participant
https://crrev.com/c/6551784 and https://crrev.com/c/6546841 modified
//base:build_date to simply copy the generated timestamp header file
from the default toolchain and avoid having to redo the work in every
toolchain. The changes also introduced an assertion to ensure that was
the case.
In AOSP builds, that assertion fails because it uses copied_base with
the old code. This PR fixes this by reusing the same mechanism that is
already in place at //base:build_date_internal.
Bug: 495203133